Repository navigation
fix(schema): emit INDEXMISSING after the vector attribute block - #771
RaphaelFakhri wants to merge 2 commits into
Conversation
|
Hi, thanks for your contribution! Can you rebase onto the latest |
A vector field with index_missing=True put INDEXMISSING into the
algorithm attribute dict, so create() failed with "Invalid input of
type: 'bool'", and the server rejects the token inside the attribute
block anyway ("Bad arguments for algorithm"). Pass index_missing to the
redis-py VectorField instead, which appends INDEXMISSING after the block
as FT.CREATE expects.
723e8b1 to
c5213f9
Compare
|
Rebased onto the latest |
vishal-bala
left a comment
There was a problem hiding this comment.
Thanks for the contribution! This looks good, just a couple minor points and one additional spot for scope
| self.attrs.algorithm, | ||
| field_data, | ||
| as_name=as_name, | ||
| index_missing=self.attrs.index_missing, |
There was a problem hiding this comment.
Now that a vector field can be created with index_missing, from_existing should recover it too. Otherwise a from_existing → create(overwrite=True) round trip still quietly drops the modifier. The parser skips the trailing INDEXMISSING in the vector branch of convert_index_info_to_schema (redisvl/redis/connection.py, the flags loop around line 408), and tests/unit/test_convert_index_info.py:277-279 pins that, with a comment that's now out of date ("can't be recreated with it yet"). Could you pick it up there, flip that assertion to expect index_missing: True, and check it against the four client configs in test_client_from_existing_and_batch_search?
There was a problem hiding this comment.
Done in 064fd78. parse_vector_attrs now sets index_missing: True when INDEXMISSING follows the vector params. That covers the trailing token in the list response and flags in the dict responses, including the packed RESP2 form. The assertion in test_convert_index_info.py now expects index_missing: True, and the outdated comment is gone.
The vector field in test_client_from_existing_and_batch_search (sync and async) now sets index_missing: True, so its reopened.schema == index.schema check covers the round trip. All four client configs pass against redis:8.4 with redis-py 8.1. Without the parser change, all four fail.
| field_data = flat_field_with_missing.attrs.field_data | ||
| assert "INDEXMISSING" in field_data | ||
| assert field_data["INDEXMISSING"] == True | ||
| assert "INDEXMISSING" not in field.attrs.field_data |
There was a problem hiding this comment.
Two trims: this field_data assertion checks an internal detail that the redis_args() assertions below already cover, and the "without index_missing" half at the end passed before the fix. While you're here, test_vector_fields_redis_creation_with_index_missing (line 359) only checks isinstance and .name and passed while the bug was live, so this test replaces it.
There was a problem hiding this comment.
Both trims are done, and test_vector_fields_redis_creation_with_index_missing is removed in 064fd78.
| index.delete(drop=True) | ||
|
|
||
| @pytest.mark.parametrize("algorithm", ["flat", "hnsw"]) | ||
| def test_vector_field_index_missing(self, client, redis_url, worker_id, algorithm): |
There was a problem hiding this comment.
The unit test already covers all three algorithms, so this only needs to prove the server accepts the modifier and answers ismissing. Could you fold it into test_indexmissing_enables_ismissing_query (line 210) as a second parametrised case (text, plus vector with HNSW) instead of adding a new test?
There was a problem hiding this comment.
Folded into test_indexmissing_enables_ismissing_query as text and vector-hnsw cases in 064fd78. The separate test is removed.
A vector field can now be created with index_missing, so read the INDEXMISSING modifier back from FT.INFO too. Otherwise a from_existing and create(overwrite=True) round trip drops it.
Motivation
With
index_missing: Trueon a vector field,SearchIndex.create()fails.field_dataput"INDEXMISSING": Trueinside the vector attribute block, so redis-py raisesInvalid input of type: 'bool', and the server rejects the token there anyway (Bad arguments for algorithm HNSW: INDEXMISSING).FT.CREATEexpectsINDEXMISSINGafter the attribute block, which is where redis-py places it for itsindex_missingkeyword. The review on #742 described this failure.Fixes #770.
Changes
redisvl/schema/fields.py: dropINDEXMISSINGfromBaseVectorFieldAttributes.field_data, and passindex_missing=self.attrs.index_missingto the redis-pyVectorFieldinFlatVectorField,HNSWVectorFieldandSVSVectorField.as_redis_field(). This matches how the other field types handle the modifier.tests/unit/test_fields.py: replace the test that assertedINDEXMISSINGinfield_datawith a parametrized test (FLAT, HNSW, SVS-VAMANA) that checksINDEXMISSINGfollows the attribute block inredis_args().tests/integration/test_field_modifier_ordering_integration.py: create FLAT and HNSW indexes with a vectorindex_missingfield and check thatismissing(@embedding)returns only the document without the vector.from_existingstill doesn't recoverindex_missingon vector fields (see the comment intests/unit/test_convert_index_info.py). That can follow separately now that the field can be recreated.Release Notes
Vector fields declared with
index_missing: truecan now be created, andismissing(@field)queries work on them.Review record
This change was written with Claude Code (Claude Opus 5.5) and reviewed by a human before submission.
Reviewed at
c5213f9by Claude Code (Claude Opus 5.5) withpre-pr-review.Lenses: correctness (always); tests (adds unit and integration tests); Redis behaviour (changes where
FT.CREATEgetsINDEXMISSING). Skipped: security (no query-building, MCP or destructive paths); design (small fix, no API change); docs (no docs or docstrings change).INDEXMISSINGfromfield_datacould break a caller that reads it (no caller outsidefields.pyreadsfield_data); theindex_missingkeyword might not exist on the redis-py floor (redis-py 6.3.0 accepts it and emitsINDEXMISSINGafter the block, checked directly); SVS-VAMANA has no integration case (the unit test covers its argument order, and the code path is shared with FLAT and HNSW).Verified:
pytest tests/unit/test_fields.py tests/integration/test_field_modifier_ordering_integration.py tests/unit/test_convert_index_info.py(85 passed, Redis 8.4); same two test files withfields.pyfrommain(5 failed, the new cases);pytest tests/unit --ignore=tests/unit/test_mcp(1345 passed, 1 skipped);make check-types(no issues);blackandisort --profile blackon the changed files (no changes).Docs consulted: https://redis.io/docs/latest/commands/ft.create/
Not checked: cluster mode; SVS-VAMANA against a live server; Redis versions other than 8.4.
Note
Medium Risk
Changes index creation and schema recovery for vector fields; incorrect FT.CREATE ordering previously broke create(), but round-trip behavior for existing indexes now differs slightly.
Overview
Fixes
SearchIndex.create()failures when vector fields useindex_missing: true.INDEXMISSINGwas incorrectly placed inside the vectorfield_datablock; it is now passed via redis-py’sindex_missingkeyword onRedisVectorField(FLAT, HNSW, SVS-VAMANA), matching lexical fields andFT.CREATEargument order.convert_index_info_to_schemanow parsesINDEXMISSINGfrom vectorFT.INFOoutput (RESP2 list and redis-py 8flags) and setsindex_missing: trueon recovered schemas, sofrom_existinground-trips the flag.Tests assert
INDEXMISSINGappears after the attribute count inredis_args(), extendismissing()coverage to HNSW vectors, and addindex_missingon vector fields infrom_existingintegration cases.Reviewed by Cursor Bugbot for commit 064fd78. Bugbot is set up for automated code reviews on this repo. Configure here.